-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace ActiveSupport inflections with Utils methods #14778
Conversation
Review period will end on 2023-02-24 at 05:35:32 UTC. |
b4ee45d
to
1e16019
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work so far! Implementation looks good, just have some naming thoughts.
Library/Homebrew/utils/inflection.rb
Outdated
|
||
# Combines `stem`` with the `singular`` or `plural` suffix based on `count`. | ||
sig { params(count: Integer, stem: String, plural: String, singular: String).returns(String) } | ||
def self.number(count, stem, plural = "s", singular = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pluralise
(or pluralize
of you prefer `en_US) may be a nicer name here.
Library/Homebrew/utils/inflection.rb
Outdated
# Inflection utility methods, as a lightweight alternative to `ActiveSupport::Inflector``. | ||
# | ||
# @api private | ||
module Inflection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this matches ActiveSupport but similarly perhaps making this just Utils.pluralize
may be a nicer fit with the existing API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do, though it feels materially different from the others at the top level:
irb(main):001:0> Utils.methods(false)
=> [:safe_popen_read, :git_commit_message, :popen, :git_short_head, :git_branch, :popen_read, :popen_write, :safe_popen_write, :git_head, :rewrite_child_error, :safe_fork]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dduugg Honestly I think those ones have just ended up there because we didn't use to separate into submodules. I think the benefit of having this just be Utils::pluralize
or, probably overkill, even in or wrapped in utils.rb
as pluralize
is it makes invocations, usually interpolated, much shorter.
Library/Homebrew/utils/inflection.rb
Outdated
|
||
# Combines `stem`` with the `singular`` or `plural` suffix based on `count`. | ||
sig { params(count: Integer, stem: String, plural: String, singular: String).returns(String) } | ||
def self.number(count, stem, plural = "s", singular = "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making plural
and singular
keyword rags? Could even abbreviate to pl
and sg
if line length is a concern. I think it will be much easier to use the methods this way since remembering the order won't be necessary.
def self.number(count, stem, plural = "s", singular = "") | |
def self.number(count, stem, plural: "s", singular: "") |
I also like the pluralize
naming, and if we go with that I think it makes sense to have the stem come first. So ultimately:
def self.number(count, stem, plural = "s", singular = "") | |
def self.pluralize(stem, count, plural: "s", singular: "") |
I'll go with the majority and call it I don't have a strong opinion about parameter ordering or kwarg vs positional, so i'll go with @Rylan12's recommendation there. |
I've changed the name to PTAL @Rylan12 @MikeMcQuaid |
7a937a7
to
2394572
Compare
Review period ended. |
a8b4a6f
to
3420278
Compare
ec43833
to
4ab2f20
Compare
@MikeMcQuaid @Rylan12 PTAL
|
4ab2f20
to
7f042c6
Compare
@@ -3415,56 +3415,6 @@ class Regexp::Token < ::Struct | |||
end | |||
end | |||
|
|||
class String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This serves as a type guard to avoid introducing monkey-patch dependencies into typed code while the deletion of require "active_support/core_ext/string/inflections"
in global.rb is pending
@@ -76,7 +76,8 @@ def parse_api_response(limit = nil, last_package = "", repository:) | |||
break if (limit && outdated_packages.size >= limit) || response.size <= 1 | |||
end | |||
|
|||
puts "#{outdated_packages.size} outdated #{package_term.pluralize(outdated_packages.size)} found" | |||
package_term = package_term.chop if outdated_packages.size == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the only dynamic instance of pluralize
. I took advantage of the fact that there's an easier path from plural to singular among {formulae, casks, packages} and reversed the logic here
@@ -103,7 +103,8 @@ def self.versions_from_tags(tags, regex = nil, &block) | |||
tags.map do |tag| | |||
if regex | |||
# Use the first capture group (the version) | |||
tag.scan(regex).first&.first | |||
# This code is not typesafe unless the regex includes a capture group | |||
T.unsafe(tag.scan(regex).first)&.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a type error that appears when deleting String#first
from the active support rbi below (but I don't think this code uses that path, looking at call sites).
@@ -46,7 +46,7 @@ def self.uninstall_casks(*casks, binaries: nil, force: false, verbose: false) | |||
next if (versions = cask.versions).empty? | |||
|
|||
puts <<~EOS | |||
#{cask} #{versions.to_sentence} #{"is".pluralize(versions.count)} still installed. | |||
#{cask} #{versions.to_sentence} #{versions.count == 1 ? "is" : "are"} still installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's possible that versions.one?
makes sense in substitutions like these, but there are some edge cases i didn't want to risk
743e28d
to
2073e70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice cleanup!
Thanks again @dduugg! |
This was replaced by a method in `Utils`, but we don't really need that here either. See Homebrew/brew#14778.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Based on @MikeMcQuaid's comment in #10508 (comment) I've put together a quick replacement candidate for ActiveSupport's
String#pluralize
monkey-patch, and ported the first five instances (based ongit grep
). There are ~50 instances ofpluralize
in the repo, so I want to confirm this approach before moving forward.(Note that there are a handful of other
String
monkey-patches that I found when i remove them from the active-support rbi: five instances ofdemodulize
, one oftitleize
, and one offirst
(which may be a type error). We will need to replace these before we can remove theactive_support/core_ext/string/inflections
dependency.)(Also, after the breakages caused by #14502, I've preemptively checked
homebrew-{cask,core}
and found no instances ofpluralize
(but I didn't look for other ActiceSupport inflection methods)).